Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve change note checking #14542

Merged
merged 2 commits into from
Oct 20, 2023
Merged

Improve change note checking #14542

merged 2 commits into from
Oct 20, 2023

Conversation

esbena
Copy link
Contributor

@esbena esbena commented Oct 19, 2023

Addresses feedback in internal issue.
The change note file name checks are now more verbose explaining why they behave as they do.

@esbena esbena requested a review from a team as a code owner October 19, 2023 10:21
@esbena esbena added the no-change-note-required This PR does not need a change note label Oct 19, 2023

echo "Change notes found:"
echo "$change_note_files"

- name: Fail if the change note filename doesn't match the expected format. The file name must be of the form 'YYYY-MM-DD.md', 'YYYY-MM-DD-{title}.md', where '{title}' is arbitrary text, or released/x.y.z.md for released change-notes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This long name for the step doesn't really make sense. Maybe shorten it to a handful of words and print out some variant of the deleted text if there is an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that is an atypically long workflow step name, but I think the motivation is that the long name acts as a description at some convenient UI level.

Example.

image

A general rewrite with the more modern markdown step summaries could be worthwhile in general.

Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the screenshot. Looks fine as-is, but still slightly better to include a detailed description in the error message.

@esbena esbena merged commit 1b9b6ae into main Oct 20, 2023
7 of 8 checks passed
@esbena esbena deleted the esbena/proper-check-change-note branch October 20, 2023 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants